Skip to content

Conversation

@jungpark-mlir
Copy link
Contributor

@jungpark-mlir jungpark-mlir commented Jul 30, 2025

Enabling users to explicitly specify which regions should be preserved, uncovers additional opportunities to utilize scf.execute_region op.

More control over the IR.
Enabling users to explicitly specify which regions should be preserved, uncovers additional opportunities to utilize `scf.execute_region`.
@llvmbot
Copy link
Member

llvmbot commented Jul 30, 2025

@llvm/pr-subscribers-mlir-scf

@llvm/pr-subscribers-mlir

Author: Jungwook Park (jungpark-mlir)

Changes

More control over the IR.
Enabling users to explicitly specify which regions should be preserved, uncovers additional opportunities to utilize scf.execute_region op.


Full diff: https://github.com/llvm/llvm-project/pull/151352.diff

3 Files Affected:

  • (modified) mlir/include/mlir/Dialect/SCF/IR/SCFOps.td (+4)
  • (modified) mlir/lib/Dialect/SCF/IR/SCF.cpp (+1-1)
  • (modified) mlir/test/Dialect/SCF/canonicalize.mlir (+22)
diff --git a/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td b/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
index 2d15544e871b3..e6b83fca771a9 100644
--- a/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
+++ b/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
@@ -119,6 +119,10 @@ def ExecuteRegionOp : SCF_Op<"execute_region", [
     ```
   }];
 
+  let arguments = (ins
+    UnitAttr:$no_inline
+  );
+
   let results = (outs Variadic<AnyType>);
 
   let regions = (region AnyRegion:$region);
diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index 759e58b617578..6b7f11a3469d6 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -184,7 +184,7 @@ struct SingleBlockExecuteInliner : public OpRewritePattern<ExecuteRegionOp> {
 
   LogicalResult matchAndRewrite(ExecuteRegionOp op,
                                 PatternRewriter &rewriter) const override {
-    if (!op.getRegion().hasOneBlock())
+    if (!op.getRegion().hasOneBlock() || op.getNoInline())
       return failure();
     replaceOpWithRegion(rewriter, op, op.getRegion());
     return success();
diff --git a/mlir/test/Dialect/SCF/canonicalize.mlir b/mlir/test/Dialect/SCF/canonicalize.mlir
index 12d30e17f4a8f..ae02144f06f32 100644
--- a/mlir/test/Dialect/SCF/canonicalize.mlir
+++ b/mlir/test/Dialect/SCF/canonicalize.mlir
@@ -1461,6 +1461,28 @@ func.func @execute_region_elim() {
 
 // -----
 
+// CHECK-LABEL: func @execute_region_elim_noinline
+func.func @execute_region_elim_noinline() {
+  affine.for %i = 0 to 100 {
+    "test.foo"() : () -> ()
+    %v = scf.execute_region -> i64 {
+      %x = "test.val"() : () -> i64
+      scf.yield %x : i64
+    } {no_inline}
+    "test.bar"(%v) : (i64) -> ()
+  }
+  return
+}
+
+// CHECK-NEXT:     affine.for %arg0 = 0 to 100 {
+// CHECK-NEXT:       "test.foo"() : () -> ()
+// CHECK-NEXT:       scf.execute_region
+// CHECK-NEXT:       %[[VAL:.*]] = "test.val"() : () -> i64
+// CHECK-NEXT:       scf.yield %[[VAL]] : i64
+// CHECK-NEXT:     }
+
+// -----
+
 // CHECK-LABEL: func @func_execute_region_elim
 func.func @func_execute_region_elim() {
     "test.foo"() : () -> ()

Copy link
Collaborator

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please handle this in the assembly format directly as a property instead of letting this part of the attribute dictionary.

Also I would rather see the keyword on the first line.

@matthias-springer
Copy link
Member

Can you also add a sentence to the description?

@jungpark-mlir
Copy link
Contributor Author

Please handle this in the assembly format directly as a property instead of letting this part of the attribute dictionary.

@joker-eph Please see if I understand this correctly,
scf.execute_region uses custom assembly and it already parse/print optional attributes, so only need to document in the description?
And use op->getAttr("no_inline") and setAttr when needed?

@joker-eph
Copy link
Collaborator

Actually you're missing the distinction between inherent and discardable attributes. We can already have discardable attributes, but here you're adding an inherent one by declaring it in ODS, so I wouldn't want it to be part of the discardable ones in the syntax.
So I'd like the actual printing/parsing code to parse an optional keyword "no_inline" on the first line before the body.

@jungpark-mlir
Copy link
Contributor Author

Actually you're missing the distinction between inherent and discardable attributes. We can already have discardable attributes, but here you're adding an inherent one by declaring it in ODS, so I wouldn't want it to be part of the discardable ones in the syntax. So I'd like the actual printing/parsing code to parse an optional keyword "no_inline" on the first line before the body.

Got it!

@github-actions
Copy link

github-actions bot commented Jul 30, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@jungpark-mlir jungpark-mlir requested a review from joker-eph July 30, 2025 20:16
Comment on lines 90 to 92
Canonicalizer inlines an ExecuteRegionOp into its parent if it only contains
one block or its parent can contain multiple blocks, 'no_inline' attribute
can be set to prevent an ExecuteRegionOp from being inlined.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird to describe an op semantics in terms of a specific pass.

Suggested change
Canonicalizer inlines an ExecuteRegionOp into its parent if it only contains
one block or its parent can contain multiple blocks, 'no_inline' attribute
can be set to prevent an ExecuteRegionOp from being inlined.
The optional 'no_inline' flag can be set to request the ExecuteRegionOp to be
preserved as much as possible and not being inlined in the parent block until
an explicit lowering step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, thanks!

Copy link
Collaborator

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@jungpark-mlir jungpark-mlir merged commit 90e710b into llvm:main Jul 31, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants